Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Send an "edited" event to all viewers when an "edit" event occurred in the streaming plugin #3000

Merged
merged 3 commits into from
Jun 17, 2022
Merged

Send an "edited" event to all viewers when an "edit" event occurred in the streaming plugin #3000

merged 3 commits into from
Jun 17, 2022

Conversation

amoizard
Copy link
Contributor

When someone sends an "edit" request to modify a streaming plugin mountpoint, viewers are not aware of the "edit".

This PR is used within our deployments so that all viewers will receive an "edited" event containing the id, permanent and metadata parameters. There are 2 purposes: being aware of a change for the mountpoint definition and retrieving immediately the updated metadata, if any.

{
"streaming" : "edited",
"id" : ,
"permanent" : <true|false, depending on whether the changes were saved to configuration file or not>
"metadata" : "<updated metadata for the mountpoint; optional>",
}

Feel free to send any comments.
Kind Regards,

Send an "edited" event to all viewers when an "edit" event occurred s…
9831398 
…o all viewers receive the new metadata

Signed-off-by: Aymeric Moizard <[email protected]>
@januscla
Copy link

Thanks for your contribution, @amoizard! Please make sure you sign our CLA, as it's a required step before we can merge this.

@lminiero
Copy link
Member

Thanks for the contribution, but I'd rather this functionality was optional, possibly as an argument passed to edit itself and false by default for backwards compatibility. There may be reasons why a mass notification is not wanted, and I don't want to force it on people.

On the PR itself, the documentation says "permanent" is sent as well, but that's not the case: in the code you only push "metadata". I'm personally ok with that, as I don't see why users would care about a change being permanent or not. Besides, I'd only send an update on "metadata" if a new "metadata" was provided in the first place: if you send an edit that changes other things and not "metadata", why send an event to everyone about something that stayed the same?

Send an "edited" event to all viewers when an "edit" event occurred s…
52865c9 
…o all viewers receive the new metadata

* The edit request have an option to enable the event "edited_event"
* The edited event is only sent if the edit contains "edited_event=true" AND if the metadata has been updated
* Documentation updated

Signed-off-by: Aymeric Moizard <[email protected]>
@amoizard
Copy link
Contributor Author

Hi Lorenzo,

Following your advise, I pushed another commit:

  • I have added an optional edited_event (false by default) in the edit request.
  • I have also made sure that the edited event will not be sent if there is no update of the metadata parameter.
  • I have fixed the documentation by removing the extra permanent parameter.
  • I finally updated the documentation with the new edited_event parameter for the edit request.

Thanks!

lminiero
lminiero reviewed Jun 17, 2022
View reviewed changes
Copy link
Member

@lminiero lminiero left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixes! I've noticed a few other things, which you can find below. Mostly editorial, so nothing big!

src/plugins/janus_streaming.c Outdated Show resolved Hide resolved
src/plugins/janus_streaming.c Outdated Show resolved Hide resolved
src/plugins/janus_streaming.c Show resolved Hide resolved
src/plugins/janus_streaming.c Outdated Show resolved Hide resolved
src/plugins/janus_streaming.c Outdated Show resolved Hide resolved
src/plugins/janus_streaming.c Outdated Show resolved Hide resolved
Send an "edited" event to all viewers when an "edit" event occurred s…
d6b8f7e 
…o all viewers receive the new metadata

* fix typo and documentation text
* add edited_event to the object that performs the automated valitation checks on incoming JSON payloads
* fix coding style
* rename a variable to avoid shadowing

NOTE: add missing new_metadata to the object that performs the automated valitation checks on incoming JSON payloads

Signed-off-by: Aymeric Moizard <[email protected]>
@amoizard
Copy link
Contributor Author

amoizard commented Jun 17, 2022 •
edited
Loading

The newer commit should take into account all your comments.

I also added new_metadata to the object that performs the automated validation checks on incoming JSON payloads.
It seems it was also missing there?

@lminiero lminiero added the multistream Related to Janus 1.x label Jun 17, 2022
@lminiero
Copy link
Member

I also added new_metadata to the object that performs the automated validation checks on incoming JSON payloads.
It seems it was also missing there?

Thanks and good catch!

@lminiero
Copy link
Member

Thanks again, merging then 👍

@lminiero lminiero merged commit 7a1fe4b into meetecho:master Jun 17, 2022
@amoizard amoizard deleted the streaming-edited-event branch June 17, 2022 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lminiero lminiero lminiero left review comments

Assignees
No one assigned
Labels
multistream Related to Janus 1.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@amoizard @januscla @lminiero